Feat/budget limits#82
Conversation
|
Warning Review limit reached
More reviews will be available in 47 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds full CRUD (create, read, update, delete) support for budget limits in LamPyrid. It introduces data models, Firefly HTTP client methods, service-layer business logic, MCP tool registration, and comprehensive test coverage spanning unit and integration tests. ChangesBudget Limit CRUD Operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
==========================================
- Coverage 97.66% 97.53% -0.14%
==========================================
Files 20 20
Lines 3130 3248 +118
==========================================
+ Hits 3057 3168 +111
- Misses 73 80 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lampyrid/services/budgets.py (1)
281-287: ⚡ Quick winPaired start/end dates are already enforced by the request models
_resolve_periodfalls back to the current month unless bothstart_dateandend_dateare non-None(src/lampyrid/services/budgets.py:281-287). However, the request models that reach this helper (SetBudgetLimitRequest/DeleteBudgetLimitRequest, via_BudgetLimitRequestBase) reject mismatched inputs by raisingValueError('Provide both start_date and end_date, or neither.')when only one is provided (src/lampyrid/models/lampyrid_models.py:833-840), so the “silently discarded single date” path should be unreachable.Defense-in-depth: consider making
_resolve_periodexplicitly raise when exactly one argument is non-Noneinstead of defaulting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lampyrid/services/budgets.py` around lines 281 - 287, _resolve_period currently defaults to the current month when one of start_date/end_date is provided and the other is None; update it to raise a ValueError when exactly one of start_date or end_date is non-None to enforce the same validation already present in the request models (SetBudgetLimitRequest, DeleteBudgetLimitRequest via _BudgetLimitRequestBase). Locate the _resolve_period function and replace the silent fallback branch with an explicit check that raises ValueError('Provide both start_date and end_date, or neither.') when (start_date is None) != (end_date is None), otherwise keep the existing behavior for both provided or both None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lampyrid/models/lampyrid_models.py`:
- Around line 790-791: The model currently substitutes silent defaults for
required fields by using budget_id=attrs.budget_id or '' and
amount=float(attrs.amount) if attrs.amount is not None else 0.0; update the
model to preserve None instead of masking missing data by changing the field
type annotations to Optional[str] and Optional[float] and remove the fallback
substitutions in the factory/constructor code that consumes attrs (leave
budget_id as attrs.budget_id and set amount to float(attrs.amount) only when
attrs.amount is not None, otherwise None); ensure the class-level type hints and
the classmethod that builds instances from attrs are updated together to reflect
Optional types.
In `@tests/integration/test_budgets.py`:
- Around line 469-489: The test test_delete_budget_limit creates a budget limit
via mcp_client.call_tool('set_budget_limit') but doesn't register it with the
budget_limit_cleanup fixture, so a failed delete/assert can leak state; after
the set_budget_limit call (the created limit), register that created limit with
the budget_limit_cleanup fixture (e.g., call budget_limit_cleanup.register or
the fixture's add method with the created limit id/period) so the fixture will
remove the limit on teardown if the in-test deletion fails.
---
Nitpick comments:
In `@src/lampyrid/services/budgets.py`:
- Around line 281-287: _resolve_period currently defaults to the current month
when one of start_date/end_date is provided and the other is None; update it to
raise a ValueError when exactly one of start_date or end_date is non-None to
enforce the same validation already present in the request models
(SetBudgetLimitRequest, DeleteBudgetLimitRequest via _BudgetLimitRequestBase).
Locate the _resolve_period function and replace the silent fallback branch with
an explicit check that raises ValueError('Provide both start_date and end_date,
or neither.') when (start_date is None) != (end_date is None), otherwise keep
the existing behavior for both provided or both None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3711c58e-ef30-490d-9835-c1de8530413f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/lampyrid/clients/firefly.pysrc/lampyrid/models/lampyrid_models.pysrc/lampyrid/services/budgets.pysrc/lampyrid/tools/budgets.pytests/conftest.pytests/fixtures/budgets.pytests/integration/test_budgets.pytests/unit/test_budgets_service.pytests/unit/test_tool_annotations.py
- BudgetLimit.from_budget_limit_read: drop silent defaults for the required budget_id/amount fields so missing API data fails loudly instead of becoming '' / 0.0 - _resolve_period: raise when exactly one of start/end is provided (defense-in-depth; mirrors request-model validation) - test_delete_budget_limit: register the created limit with budget_limit_cleanup as a teardown safety net
a5563e1 to
b24a44e
Compare
Implements #74